Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add assertions signature for TypeScript #10543

Merged

Conversation

tanhauhau
Copy link
Member

@tanhauhau tanhauhau commented Oct 12, 2019

Q                       A
Fixed Issues? Fixes #10527
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

Should I make changes to the docs manually, or wait for babel/website#2080?

@tanhauhau tanhauhau changed the title add asserts predicate add asserts predicate for TypeScript Oct 12, 2019
@tanhauhau tanhauhau changed the title add asserts predicate for TypeScript add assertions signature for TypeScript Oct 13, 2019
@nicolo-ribaudo nicolo-ribaudo added this to the TypeScript 3.7 milestone Oct 13, 2019
@nicolo-ribaudo nicolo-ribaudo added pkg: parser PR: New Feature 🚀 A type of pull request used for our changelog categories labels Oct 13, 2019
fields: {
assertsModifier: validateOptionalType("TSAssertsKeyword"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a boolean, since the only needed information is if it is present or not.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was following the TypeScript AST.
But I understand Babel AST doesn't have to be the same as Typescript AST.

@@ -904,21 +904,34 @@ export default (superClass: Class<Parser>): Class<Parser> =>
const t: N.TsTypeAnnotation = this.startNode();
this.expect(returnToken);

const assertsModifier = this.tsTryParse(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this could be parser without using tryParse: it would be possible to parse the assert identifier and then convert it to a type annotation if the next token is not another identifier.

On the other hand, we use tryParse everywhere when parsing TypeScript. I think that it's better to be consistent here, and then the whole TS parser can be optimized on other PRs.

@@ -73,12 +73,12 @@
},
"typeAnnotation": {
"type": "TSTypePredicate",
"start": 10,
"start": 8,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@nicolo-ribaudo
Copy link
Member

The CircleCI failure is unrelated and already fixed on master.

@tanhauhau tanhauhau force-pushed the tanhauhau/typescript-asserts-predicate branch from 544d494 to 128cd3d Compare October 16, 2019 13:54
@nicolo-ribaudo nicolo-ribaudo added the PR: Needs Review A pull request awaiting more approvals label Oct 17, 2019
Copy link
Member

@eventualbuddha eventualbuddha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I'm reviewing this because of this: https://babeljs.slack.com/archives/C062RC35M/p1571345121030900)

Having read the blog post on TypeScript 3.7 and played around with it in the local REPL, this seems sane to me.

@nicolo-ribaudo nicolo-ribaudo added PR: Ready to be Merged A pull request with already two approvals, but waiting for the next minor release and removed PR: Needs Review A pull request awaiting more approvals labels Oct 17, 2019
@bradzacher
Copy link
Contributor

bradzacher commented Oct 29, 2019

Hi there!

We on typescript-eslint added support for this to our parser a few weeks ago (in typescript-eslint/typescript-eslint#1045)

Could we look to align the AST representations?
Specifically we chose to go with asserts: boolean instead of assertsModifier: boolean.

I got alerted to this via typescript-eslint/typescript-eslint#1158.
It'd be great if we can work together in future to ensure we are aligned on our AST representations.

@nicolo-ribaudo
Copy link
Member

I'm 100% ok with it. We are going to release a new Babel version soon; would you be able to prepare a quick PR?

@thorn0
Copy link
Contributor

thorn0 commented Oct 29, 2019

quick PR

I'll do it

@existentialism
Copy link
Member

@thorn0 thanks!

JLHwung pushed a commit that referenced this pull request Oct 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: parser PR: New Feature 🚀 A type of pull request used for our changelog categories PR: Ready to be Merged A pull request with already two approvals, but waiting for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TypeScript assertion signatures
6 participants